Switch HTTP client from hackney to finch #758

savhappy · 2024-07-29T20:13:39Z

switch from Hackney to Finch
alter tests to pass with Finch
closes Switch from Hackney to Finch as the default HTTP client #724

whatyouhide

Great start!! Left a few comments to address 🙃

lib/mix/tasks/sentry.send_test_event.ex

lib/sentry/config.ex

whatyouhide · 2024-07-30T10:29:59Z

lib/sentry/config.ex

@@ -253,30 +253,30 @@ defmodule Sentry.Config do
      The maximum number of attempts to send an event to Sentry.
      """
    ],
-    hackney_opts: [


We need to keep hackney_opts and deprecate them if we want this to not be a breaking change. NimbleOptions supports deprecating options, check out the docs for that.

This also applies to the options below.

Does this mean we need to keep the option:

client: [ type: :atom, type_doc: "`t:module/0`", default: Sentry.HackneyClient, doc: """ """ ]

and the module HackneyClient?? @whatyouhide

lib/sentry/finch_client.ex

whatyouhide · 2024-07-30T10:30:37Z

lib/sentry/finch_client.ex

+  HTTP client, you'll have to implement your own `Sentry.HTTPClient`. See the
+  documentation for `Sentry.HTTPClient` for more information.
+
+  Finch is built on top of NimblePool. If you need to set other pool configuration options,


Can you link to nimble_pool's repo here?

whatyouhide · 2024-07-30T10:31:03Z

lib/sentry/finch_client.ex

+  see "Pool Configuration Options" in the source code for details on the possible map values.
+  [finch configuration options](https://github.com/sneako/finch/blob/main/lib/finch.ex)


We shouldn't point people to source code, that can change (especially main). Let's link to Finch for docs

lib/sentry/finch_client.ex

whatyouhide · 2024-07-30T10:32:21Z

lib/sentry/config.ex

+  @spec finch_opts() :: keyword()
+  def finch_opts, do: fetch!(:finch_opts)


Where do we use these?

aselder · 2025-02-11T20:18:12Z

Any progress on this?

A CVE (with no patched version) was issued against Hackney today: benoitc/hackney#751

savhappy · 2025-02-11T20:58:15Z

@aselder we backlogged this yesterday but perhaps we should continue with revisions and make the switch? @whatyouhide @sl0thentr0py

A quick fix is to configure your own HTTP client.

whatyouhide · 2025-02-12T07:30:35Z

@savhappy yeah let's pursue this. Where we at? What we need to do?

savhappy · 2025-02-13T17:05:30Z

@whatyouhide it's in good shape after I fix the conflicts. I'll rerequest a review when ready

savhappy · 2025-02-15T18:24:19Z

@whatyouhide when we deprecate the :hackney_opts in the config file we get a ton of warnings from NimbleOptions when it tries to validate the config opts. So our tests are cluttered with these warnings:

warning: :hackney_opts option is deprecated. Use Finch instead as default client. (nimble_options 1.1.1) lib/nimble_options.ex:575: NimbleOptions.validate_value/3 (nimble_options 1.1.1) lib/nimble_options.ex:561: NimbleOptions.validate_option/3 (nimble_options 1.1.1) lib/nimble_options.ex:542: NimbleOptions.reduce_options/2 (elixir 1.17.2) lib/enum.ex:4858: Enumerable.List.reduce/3 (elixir 1.17.2) lib/enum.ex:2585: Enum.reduce_while/3 (nimble_options 1.1.1) lib/nimble_options.ex:535: NimbleOptions.validate_options/2 (nimble_options 1.1.1) lib/nimble_options.ex:508: NimbleOptions.validate_options_with_schema_and_path/3 (sentry 10.8.1) lib/sentry/config.ex:505: Sentry.Config.validate!/1 (sentry 10.8.1) lib/sentry/config.ex:661: Sentry.Config.put_config/2 (sentry 10.8.1) test/support/test_helpers.ex:32: anonymous fn/2 in Sentry.TestHelpers.put_test_config/1 (elixir 1.17.2) lib/enum.ex:2531: Enum."-reduce/3-lists^foldl/2-0-"/3 (sentry 10.8.1) test/support/test_helpers.ex:24: Sentry.TestHelpers.put_test_config/1 test/sentry/test_test.exs:13: Sentry.TestTest.__ex_unit_setup_0/1 test/sentry/test_test.exs:1: Sentry.TestTest.__ex_unit__/2 (ex_unit 1.17.2) lib/ex_unit/runner.ex:478: ExUnit.Runner.exec_test_setup/2 (ex_unit 1.17.2) lib/ex_unit/runner.ex:411: anonymous fn/2 in ExUnit.Runner.spawn_test_monitor/4 (stdlib 5.2.3) timer.erl:270: :timer.tc/2 (ex_unit 1.17.2) lib/ex_unit/runner.ex:407: anonymous fn/6 in ExUnit.Runner.spawn_test_monitor/4

How do I quiet them or deprecate in a more subtle way?

whatyouhide · 2025-02-17T06:44:57Z

How do I quiet them or deprecate in a more subtle way?

Start with

hackney_opts: [
  type: ...,
  ...
] ++ if(Mix.env() == :test, do: [], else: [deprecated: "..."])

Co-authored-by: Andrea Leopardi <[email protected]>

:x

jjcarstens

🎉 So excited for this! ❤️

Sorry for the random review. We were starting on a quick HTTP client to replace Hackney and saw this was in progress, so I am very interested in helping push it through since it is so close. Happy to contribute changes if needed 🤠

jjcarstens · 2025-03-02T16:10:53Z

mix.exs

-      xref: [exclude: [:hackney, :hackney_pool, Plug.Conn, :telemetry]],
-      aliases: aliases()
+      xref: [exclude: [Finch, Plug.Conn, :telemetry]],
+      aliases: [aliases()]


Suggested change

aliases: [aliases()]

aliases: aliases()

@savhappy The mix test.integrations task in CI is unavailable because this was changed to a list when the aliases/0 function already returns a list. Reverting back should get you running

jjcarstens · 2025-03-02T16:23:08Z

lib/sentry/config.ex

@@ -298,32 +298,64 @@ defmodule Sentry.Config do
      The maximum number of attempts to send an event to Sentry.
      """
    ],
-    hackney_opts: [
+    finch_opts: [
      type: :keyword_list,
      default: [pool: :sentry_pool],


I don't believe pool: :sentry_pool is not a valid option for Finch? But it also looks like finch_opts is not used. Maybe it is best to shift to allowing a separation between Finch request_opts vs start_link opts? Sentry may not need to do intense validation since Finch also uses nimble_options under the hood

Thanks @jjcarstens for the review 💜

You're right! :sentry_pool isn't valid. The Finch config options were just placeholder until we decide what options to expose. Which of the pool options do we want to support? @whatyouhide or @jjcarstens thoughts?
Maybe it makes sense to have a catch all like you (@jjcarstens) mentioned configuration option without the need to handle validation.

finch_pool_opts: [ type: :keyword_list, default: [size: 50, conn_max_idle_time: 5000], doc: """ Start link pool ptions to be passed to `finch`.... """ ]

jjcarstens · 2025-03-02T16:24:10Z

lib/sentry/hackney_client.ex

The :hackney_opts was switched to soft deprecation, but this Hackney client was totally removed which I assume is equally as breaking. Is there a specific reason not to leave it in until hackney is hard deprecated?

jjcarstens · 2025-03-02T16:24:42Z

config/config.exs

@@ -6,7 +6,7 @@ if config_env() == :test do
    tags: %{},
    enable_source_code_context: true,
    root_source_code_paths: [File.cwd!()],
-    hackney_opts: [recv_timeout: 50, pool: :sentry_pool],
+    finch_opts: [recv_timeout: 50],


Suggested change

finch_opts: [recv_timeout: 50],

finch_opts: [receive_timeout: 50],

According to t:Finch.request_opt(), this should be :receive_timeout

whatyouhide · 2025-03-03T07:45:26Z

@savhappy do you have an ETA for when you'd be able to make some progress on this?

savhappy · 2025-03-03T09:11:00Z

@whatyouhide working on this today. I need to add back in the Hackney client like @jjcarstens mentions so no breaking changes are introduced and await feedback on finch options to expose. After this, ready for another review.

savhappy requested a review from whatyouhide July 29, 2024 20:13

savhappy mentioned this pull request Jul 29, 2024

Switch from Hackney to Finch as the default HTTP client #724

Open

whatyouhide reviewed Jul 30, 2024

View reviewed changes

savhappy changed the title ~~Sav/switch hackney to finch~~ Switch HTTP client from hackney to finch Aug 6, 2024

whatyouhide added this to the 11.0.0 milestone Aug 20, 2024

whatyouhide mentioned this pull request Jan 3, 2025

Add support for Req http client (draft) #846

Closed

savhappy force-pushed the sav/Switch-hackney-to-finch branch 3 times, most recently from 36a8c52 to fbd58f0 Compare February 13, 2025 07:01

savhappy force-pushed the sav/Switch-hackney-to-finch branch from b20ab52 to c479fb2 Compare February 15, 2025 18:21

savhappy force-pushed the sav/Switch-hackney-to-finch branch from c479fb2 to f13edfd Compare February 15, 2025 18:59

savhappy force-pushed the sav/Switch-hackney-to-finch branch from 9f4dff2 to ee7e3e5 Compare February 21, 2025 06:10

savhappy force-pushed the sav/Switch-hackney-to-finch branch from c925a56 to c7d2df9 Compare February 28, 2025 23:36

savhappy and others added 11 commits February 28, 2025 18:37

updated .elixir_ls

fb76cdd

add initial Finch file and test

3bb8a51

replace config hackney with finch

357fa08

correct test and doc

ebe8186

dialyzer

5602bac

Update lib/mix/tasks/sentry.send_test_event.ex

ce54a5d

Co-authored-by: Andrea Leopardi <[email protected]>

Update lib/sentry/config.ex

9eddb76

Co-authored-by: Andrea Leopardi <[email protected]>

Update lib/sentry/finch_client.ex

11f2d52

Co-authored-by: Andrea Leopardi <[email protected]>

Update lib/sentry/finch_client.ex

26f6c73

Co-authored-by: Andrea Leopardi <[email protected]>

fix test with vocab change and keep hackney options

56aa853

fix ups

9a5d14a

savhappy added 2 commits February 28, 2025 19:23

add back in deprecated hackney_opts

1d61bf4

:x

quiet depracated for Hackney

8f3fc16

savhappy force-pushed the sav/Switch-hackney-to-finch branch from c7d2df9 to 8f3fc16 Compare March 1, 2025 00:30

jjcarstens reviewed Mar 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch HTTP client from hackney to finch #758

Switch HTTP client from hackney to finch #758

savhappy commented Jul 29, 2024 •

edited

Loading

whatyouhide left a comment

whatyouhide Jul 30, 2024

savhappy Jul 30, 2024 •

edited

Loading

whatyouhide Jul 30, 2024

whatyouhide Jul 30, 2024

whatyouhide Jul 30, 2024

aselder commented Feb 11, 2025

savhappy commented Feb 11, 2025 •

edited

Loading

whatyouhide commented Feb 12, 2025

savhappy commented Feb 13, 2025

savhappy commented Feb 15, 2025

whatyouhide commented Feb 17, 2025 •

edited

Loading

jjcarstens left a comment

jjcarstens Mar 2, 2025

jjcarstens Mar 2, 2025

savhappy Mar 3, 2025 •

edited

Loading

jjcarstens Mar 2, 2025

jjcarstens Mar 2, 2025

whatyouhide commented Mar 3, 2025

savhappy commented Mar 3, 2025

		see "Pool Configuration Options" in the source code for details on the possible map values.
		[finch configuration options](https://github.com/sneako/finch/blob/main/lib/finch.ex)

		@spec finch_opts() :: keyword()
		def finch_opts, do: fetch!(:finch_opts)

	finch_opts: [recv_timeout: 50],
	finch_opts: [receive_timeout: 50],

Switch HTTP client from hackney to finch #758

Are you sure you want to change the base?

Switch HTTP client from hackney to finch #758

Conversation

savhappy commented Jul 29, 2024 • edited Loading

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savhappy Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aselder commented Feb 11, 2025

savhappy commented Feb 11, 2025 • edited Loading

whatyouhide commented Feb 12, 2025

savhappy commented Feb 13, 2025

savhappy commented Feb 15, 2025

whatyouhide commented Feb 17, 2025 • edited Loading

jjcarstens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savhappy Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide commented Mar 3, 2025

savhappy commented Mar 3, 2025

savhappy commented Jul 29, 2024 •

edited

Loading

savhappy Jul 30, 2024 •

edited

Loading

savhappy commented Feb 11, 2025 •

edited

Loading

whatyouhide commented Feb 17, 2025 •

edited

Loading

savhappy Mar 3, 2025 •

edited

Loading